Index images by RepoDigests in LocalImagesCache, not just RepoTags - #11948
Index images by RepoDigests in LocalImagesCache, not just RepoTags#11948itsmehotpants wants to merge 1 commit into
Conversation
LocalImagesCache.populateFromList() only indexed local images by their RepoTags. An image referenced only by digest (e.g. after 'docker pull image@sha256:...', or when Docker reports RepoTags as null/<none> for an otherwise-tagged image, which happens after certain rebuild/retag sequences) was never added to the cache. Testcontainers would then treat the image as absent locally and re-pull it every time, even though it was already present, defeating both the image pull policy and Ryuk/reaper image lookups by digest. populateFromList() now also reads Image::getRepoDigests() and indexes those digest-qualified names into the same cache, alongside RepoTags. An image with neither populated is still skipped, as before. Added LocalImagesCacheTest covering: digest-only images, images with both a tag and a digest, and the neither-present case. Uses the same ObjectMapper#convertValue(Map, Class) pattern already used in ReusabilityUnitTests for constructing docker-java model objects in tests, and the existing (previously unused) LocalImagesCacheAccessor for cache isolation between tests. Fixes testcontainers#1406
kdelay
left a comment
kdelay
left a comment
There was a problem hiding this comment.
I went through this one and checked it against a live daemon, since the interesting part is what the daemon actually reports. Environment: Docker 28.1.1, containerd snapshotter image store (driver-type: io.containerd.snapshotter.v1), macOS/arm64, JDK 17, PR head 948d638b vs main 2ac3c977.
The fix does change real behavior, but not through the path the description describes.
I probed LocalImagesCache.INSTANCE.get(...) (public API, so the same code path AbstractImagePullPolicy uses) with three names, on both branches:
| lookup | main |
this PR |
|---|---|---|
alpine@sha256:d9e853e8… (image pulled by digest only) |
hit | hit |
redis@sha256:09160599… (image pulled by tag, looked up by digest) |
miss | hit |
redis:alpine |
hit | hit |
Cache entries after init: 20 on main, 39 with the patch.
The first row is the scenario in the description, and it already hits on main. The reason is that this daemon puts the digest reference into RepoTags too:
RepoTags=[alpine@sha256:d9e853e8…] RepoDigests=[alpine@sha256:d9e853e8…]
So the "RepoTags is null for a digest-pulled image" premise does not hold on Docker 28.1.1 with the containerd store. What does reproduce there is the second row: an image present locally under a tag, referenced by the user as repo@sha256:…, misses the cache today and gets re-pulled on every run under PULL_ONLY_MISSING. That is a real bug and this patch fixes it. It might be worth leading with that case in the description, both because a reviewer can reproduce it in two commands and because it doesn't depend on daemon version or image store.
Two smaller things, both measured, neither blocking:
-
The new tests pin
RepoTags == null. The other shape a daemon returns for the same situation is an empty array rather thannull, and that shape isn't covered. I checked it works (Stream.of(new String[0])is empty,putAllof an empty map is a no-op), so this is a coverage gap rather than a defect. A fourth case withRepoTags == []and a populatedRepoDigestswould pin it. -
Dangling-image names now come in through a second field. I fed
populateFromLista synthetic image withRepoTags=["<none>:<none>"],RepoDigests=["<none>@<none>"]: no exception, and the cache gains<none>@<none>:latestalongside the<none>:<none>key it already had onmain(DockerImageNameparses<none>@<none>as repository<none>@<none>plus the defaultlatesttag). Harmless, and on my live daemon zero<none>keys appeared, so I'd only mention it in case you want to filter those names while you're in here.
On the test run: LocalImagesCacheTest is 3/3 green. Running the wider org.testcontainers.images.* + org.testcontainers.utility.* subset, I saw failures in AuthenticatedImagePullTest, ImagePullPolicyTest, OverrideImagePullPolicyTest and DockerfileBuildTest[4], and they are environmental here, not yours: the pull-policy ones pass standalone on your head (ImagePullPolicyTest 5/5, OverrideImagePullPolicyTest 2/2), and DockerfileBuildTest[4] fails identically on main standalone. The batch failures are registry lease errors from the containerd store (unable to lease content: lease does not exist) and vary run to run.
The @VisibleForTesting widening looks right to me, for what it's worth: maybeInitCache reaches for DockerClientFactory, so there is no way to unit-test this without either a daemon or that seam, and the class already exposes initialized and cache the same way.
2 participants
Fixes #1406.
LocalImagesCache.populateFromList()only indexed local images by theirRepoTags. An image reachable only by digest (e.g.docker pull image@sha256:...) - or a tagged image where Docker reportsRepoTagsasnull/["<none>:<none>"]after certain rebuild/retag sequences, whileRepoDigestsremains populated (see moby/moby#29157 for a concrete real-world case of this) - was never added to the cache. Testcontainers would then treat the image as absent locally and re-pull it every time, even though it was already present, defeating both the configuredImagePullPolicyand any lookup of an image by digest.Change:
populateFromList()now also readsImage::getRepoDigests()and indexes those digest-qualified names into the same cache, alongsideRepoTags. An image with neither field populated is still skipped, as before (just with an updated log message reflecting the new condition).Tests: added
LocalImagesCacheTestcovering a digest-only image, an image with both a tag and a digest, and the neither-present case. Uses the sameObjectMapper#convertValue(Map, Class)pattern already used inReusabilityUnitTestsfor constructing docker-java model objects in tests, and the existing (previously unused)LocalImagesCacheAccessortest helper for cache isolation between tests.Scope note: I did not additionally add Image ID-based lookup (mentioned in the issue's 2020 edit) since
DockerImageNameparsing doesn't cleanly represent a bare image ID (e.g.sha256:abcdef...without a repository) today - that would need its own design discussion, so I've kept this PR focused on the RepoDigests fix. Happy to look at Image ID support separately if maintainers want it.